replace hooks with smart-invoice #165
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@sshmm is attempting to deploy a commit to the Raid Guild Team on Vercel. A member of the Team first needs to authorize it. |
|
NEXT_PUBLIC_WALLETCONNECT_ID and |
remove unused variables
sshmm
force-pushed
the
use_smart_invoice_packages
branch
from
e2880c2 to
d99994a
Compare
WalkthroughThe pull request introduces significant changes to the project's frontend and backend infrastructure, focusing on updating dependencies, modifying environment variables, and refactoring hooks related to escrow functionality. The changes primarily involve migrating from Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
apps/frontend/components/Escrow/ReleaseFunds.tsx (1)
Line range hint
58-80: Remove commented-out code blocks.There are large blocks of commented-out code that should be removed rather than left in the codebase. If this code is needed for reference, consider documenting it elsewhere. This includes:
- The manual polling implementation
- The transaction link UI component
This will improve code maintainability and readability.
Also applies to: 108-118
🧹 Nitpick comments (3)
apps/frontend/components/Escrow/EscrowConfirmation.tsx (1)
Line range hint
113-134: Consider adding test coverage for error scenarios.The createInvoice function includes comprehensive error handling for user rejections and other failures. Consider adding test coverage to verify these error paths and ensure proper toast notifications.
Would you like me to help generate test cases for these scenarios?
apps/frontend/components/Escrow/WithdrawFunds.tsx (1)
Line range hint
77-82: Enhance error handling and button state management.Consider improving the error handling and button state management:
- Add error state handling and user feedback for failed transactions
- Consider additional button disabled conditions (e.g., zero balance)
+ const isZeroBalance = balance <= 0n; <Button onClick={withdrawFunds} - isDisabled={!withdrawFunds} + isDisabled={!withdrawFunds || isLoading || isZeroBalance} variant='solid' textTransform='uppercase' > - Withdraw + {isLoading ? 'Withdrawing...' : 'Withdraw'} </Button>package.json (1)
27-28: Consider keeping flexibility with caret versions.The change from
^5.0.0to exact versions (5.55.3) for @tanstack/react-query packages might miss important patches. Consider keeping the caret prefix unless exact versions are required for compatibility.{ "dependencies": { - "@tanstack/react-query": "5.55.3", - "@tanstack/react-query-devtools": "5.55.3", + "@tanstack/react-query": "^5.55.3", + "@tanstack/react-query-devtools": "^5.55.3" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/frontend/.env.example(1 hunks)apps/frontend/components/Escrow/DepositFunds.tsx(5 hunks)apps/frontend/components/Escrow/EscrowConfirmation.tsx(5 hunks)apps/frontend/components/Escrow/ReleaseFunds.tsx(2 hunks)apps/frontend/components/Escrow/WithdrawFunds.tsx(2 hunks)apps/frontend/next.config.js(1 hunks)libs/dm-utils/src/chains.ts(3 hunks)libs/dm-utils/src/web3.ts(2 hunks)libs/escrow-hooks/src/index.ts(1 hunks)libs/escrow-hooks/src/useDeposit.ts(0 hunks)libs/escrow-hooks/src/useEscrowZap.ts(0 hunks)libs/escrow-hooks/src/useInvoiceDetails.ts(2 hunks)libs/escrow-hooks/src/useRelease.ts(0 hunks)libs/escrow-hooks/src/useWithdraw.ts(0 hunks)libs/escrow-utils/src/constants.ts(1 hunks)package.json(3 hunks)
💤 Files with no reviewable changes (4)
- libs/escrow-hooks/src/useEscrowZap.ts
- libs/escrow-hooks/src/useRelease.ts
- libs/escrow-hooks/src/useDeposit.ts
- libs/escrow-hooks/src/useWithdraw.ts
🔇 Additional comments (19)
apps/frontend/components/Escrow/EscrowConfirmation.tsx (3)
93-96: LGTM! Hook integration is well-implemented.The integration of useDetailsPin and the updated useEscrowZap configuration with networkConfig and details parameters is clean and aligns with the migration objectives.
Also applies to: 105-105, 108-108
242-242: LGTM! Button states are properly managed.The button disable conditions correctly incorporate all loading states (detailsLoading, registerLoading, zapLoading) and the write function availability check.
Also applies to: 251-254
12-12: Verify if useRegister should also be migrated to @smartinvoicexyz/hooks.While useEscrowZap has been migrated to @smartinvoicexyz/hooks, useRegister is still being imported from @raidguild/escrow-hooks. This seems inconsistent with the PR's objective of replacing hooks from @raidguild/escrow-hooks.
Let's check if there's a corresponding hook in @smartinvoicexyz/hooks:
Also applies to: 14-14, 16-16
apps/frontend/components/Escrow/ReleaseFunds.tsx (2)
52-54: Verify the minimal invoice object usage.The
useReleasehook now receives only the invoice address instead of the full invoice object. While this change might be intentional for the new package, please verify:
- That this is the expected behavior
- That no other invoice properties are needed by the hook internally
Let's check the hook's implementation and usage:
11-11: Consider completing the migration to@smartinvoicexyz/hooks.The code currently mixes hooks from both
@raidguild/escrow-hooksand@smartinvoicexyz/hooks. This could lead to compatibility issues or inconsistent behavior. Consider migratingusePollSubgraphto the new package as well.Let's check if
usePollSubgraphis available in the new package:Also applies to: 13-14
✅ Verification successful
Complete the migration by using the new polling implementation
The commented-out
pollSubgraphimplementation in your file suggests you're already working on replacing theusePollSubgraphhook. Complete this migration by:
- Removing the import from
@raidguild/escrow-hooks- Uncommenting and finalizing your new implementation
This will help maintain consistency with the rest of the codebase that's being migrated to
@smartinvoicexyz/hooks.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if usePollSubgraph is available in @smartinvoicexyz/hooks rg -A 5 "export.*usePollSubgraph" node_modules/@smartinvoicexyz/hooksLength of output: 147
Script:
#!/bin/bash # Search for usePollSubgraph usage across the codebase echo "=== Searching for usePollSubgraph usage ===" rg "usePollSubgraph" echo -e "\n=== Checking package dependencies ===" # Find and show content of package.json files fd "package.json" --type f --exec cat {}Length of output: 5134
Script:
#!/bin/bash # Check the implementation of usePollSubgraph to understand its functionality echo "=== Local implementation of usePollSubgraph ===" cat libs/escrow-hooks/src/usePollSubgraph.ts # Search for similar polling functionality in the codebase echo -e "\n=== Searching for similar polling patterns ===" rg -g "!node_modules" "usePolling|pollSubgraph|pollTransaction" -A 5Length of output: 2043
apps/frontend/components/Escrow/WithdrawFunds.tsx (2)
7-12: LGTM! Import changes align with migration objectives.The changes correctly update the imports to support the migration to
@smartinvoicexyz/hooksand add toast functionality.
Line range hint
58-72: Remove or update commented transaction tracking code.The commented transaction tracking code should either be:
- Removed if it's no longer needed with the new hooks, or
- Updated to work with the new
@smartinvoicexyz/hooksimplementation if transaction tracking is still required.Let me check if transaction tracking is implemented elsewhere in the codebase:
apps/frontend/next.config.js (1)
16-16: LGTM! Addition of @smartinvoicexyz/hooks to transpilePackages.The addition is necessary to support the migration from
@raidguild/escrow-hooksto@smartinvoicexyz/hooks.libs/escrow-hooks/src/index.ts (1)
1-10: Verify replacement hooks availability.The removal of hooks (useDeposit, useEscrowZap, useRelease, useWithdraw) aligns with the migration to
@smartinvoicexyz/hooks. Let's verify the availability of equivalent hooks in the new package.✅ Verification successful
✓ Hooks migration verified successfully
All removed hooks (useDeposit, useEscrowZap, useRelease, useWithdraw) are now being imported from
@smartinvoicexyz/hooksin their respective components, confirming successful migration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all removed hooks have equivalents in @smartinvoicexyz/hooks # Check if the package is installed and examine its exports cat package.json | jq '.dependencies["@smartinvoicexyz/hooks"]' # Search for hook usage in the codebase echo "Searching for new hook imports..." rg "from '@smartinvoicexyz/hooks'" -A 1Length of output: 980
libs/dm-utils/src/web3.ts (1)
25-25: Verify environment variable updates across the codebase.The WalletConnect project ID environment variable has been renamed. Let's verify all references have been updated.
✅ Verification successful
Environment variable updates are complete, but documentation is missing
The environment variable renaming is consistent throughout the codebase. However, consider adding documentation for the required environment variables:
- Add a
.env.examplefile- Document the environment variables in the README.md
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to old environment variables echo "Checking for old WalletConnect env var..." rg "NEXT_PUBLIC_WC_PROJECT_ID" echo "\nChecking for old Alchemy env var..." rg "NEXT_PUBLIC_ALCHEMY_KEY" echo "\nChecking .env files..." fd ".env" --type fLength of output: 343
Script:
#!/bin/bash # Check new environment variable names and documentation echo "Checking new WalletConnect env var..." rg "NEXT_PUBLIC_WALLETCONNECT_ID" echo "\nChecking new Alchemy env var..." rg "NEXT_PUBLIC_ALCHEMY_ID" echo "\nChecking for example env files..." fd -g "*.env.example" -g "*.env.sample" -g "*.env.template" echo "\nChecking README files for env documentation..." fd README -e md -X grep -l "env" {} \;Length of output: 863
Script:
#!/bin/bash # Check README content for environment variable documentation echo "Checking README content..." rg -A 2 -B 2 "(NEXT_PUBLIC_WC_PROJECT_ID|NEXT_PUBLIC_ALCHEMY_KEY|NEXT_PUBLIC_WALLETCONNECT_ID|NEXT_PUBLIC_ALCHEMY_ID)" README.mdLength of output: 193
libs/dm-utils/src/chains.ts (3)
13-21: LGTM! Type-safety improvements for chains array.The changes improve type safety by making the chains array immutable with
readonlyandas const.
38-54: LGTM! Improved type definitions for network name mappings.The change from
Partial<Record>toRecord<..., string | undefined>makes the undefined case explicit and type-safe.
60-62: LGTM! Added return type annotations.The explicit return type annotations on
chainsMapandnetworkToIdMapfunctions improve type safety and code clarity.Also applies to: 74-83
libs/escrow-utils/src/constants.ts (1)
155-177: Verify Sepolia network configuration addresses.The Sepolia network configuration looks well-structured. Let's verify the contract addresses and subgraph endpoint.
apps/frontend/components/Escrow/DepositFunds.tsx (3)
17-17: LGTM! Added toast notifications for better user feedback.The addition of toast notifications improves the user experience by providing clear feedback during deposit operations.
Also applies to: 30-30, 51-51
294-297: LGTM! Improved button state management.The button state now correctly reflects the loading state and amount validation.
88-102: Verify token metadata structure matches smart-invoice requirements.The token metadata structure has been updated for the new hook. Please ensure this matches the requirements from @smartinvoicexyz/hooks.
apps/frontend/.env.example (1)
15-18: Update environment variables in deployment configurations.Environment variable names have been updated:
NEXT_PUBLIC_ALCHEMY_KEY→NEXT_PUBLIC_ALCHEMY_IDNEXT_PUBLIC_WC_PROJECT_ID→NEXT_PUBLIC_WALLETCONNECT_IDEnsure these are updated in all deployment environments.
package.json (1)
31-31: Verify compatibility with downgraded wagmi ecosystem.The coordinated downgrade of wagmi-related packages suggests a potential compatibility requirement:
- @wagmi/core: 2.14.6 → 2.13.8
- viem: 2.21.45 → 2.17.0
- wagmi: 2.12.32 → 2.12.17
Please verify if these specific versions are required by @smartinvoicexyz/hooks.
Also applies to: 62-63
| const toast = useToast(); | ||
|
|
||
| // const onSuccess = () => { | ||
| // // toast | ||
| // // close modal | ||
| // }; | ||
|
|
||
| const { writeAsync: withdrawFunds, isLoading } = useWithdraw({ invoice }); | ||
| const { writeAsync: withdrawFunds, isLoading } = useWithdraw({ | ||
| invoice: { address: invoice.address }, | ||
| onTxSuccess: () => {}, | ||
| toast, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction success handling.
The onTxSuccess callback is currently empty. Consider adding appropriate success handling, such as displaying a success message or triggering a UI update.
const { writeAsync: withdrawFunds, isLoading } = useWithdraw({
invoice: { address: invoice.address },
- onTxSuccess: () => {},
+ onTxSuccess: () => {
+ toast({
+ title: 'Withdrawal Successful',
+ description: 'Funds have been successfully withdrawn from escrow',
+ status: 'success',
+ });
+ },
toast,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toast = useToast(); | |
| // const onSuccess = () => { | |
| // // toast | |
| // // close modal | |
| // }; | |
| const { writeAsync: withdrawFunds, isLoading } = useWithdraw({ invoice }); | |
| const { writeAsync: withdrawFunds, isLoading } = useWithdraw({ | |
| invoice: { address: invoice.address }, | |
| onTxSuccess: () => {}, | |
| toast, | |
| }); | |
| const toast = useToast(); | |
| const { writeAsync: withdrawFunds, isLoading } = useWithdraw({ | |
| invoice: { address: invoice.address }, | |
| onTxSuccess: () => { | |
| toast({ | |
| title: 'Withdrawal Successful', | |
| description: 'Funds have been successfully withdrawn from escrow', | |
| status: 'success', | |
| }); | |
| }, | |
| toast, | |
| }); |
| @@ -2,7 +2,7 @@ import { getInvoice } from '@raidguild/escrow-gql'; | |||
| import { useQuery } from '@tanstack/react-query'; | |||
| import { useMemo } from 'react'; | |||
| import { Hex } from 'viem'; | |||
| import { useContractRead } from 'wagmi'; | |||
| import { useReadContract } from 'wagmi'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Update AccountLink.tsx to use useReadContract
The codebase has inconsistent usage of wagmi hooks:
useReadContractin useInvoiceDetails.tsuseContractReadin AccountLink.tsx (apps/frontend/components/Escrow/shared/AccountLink.tsx)
Both hooks are from wagmi v2, but useReadContract is the preferred pattern. Update AccountLink.tsx for consistency.
🔗 Analysis chain
Verify wagmi API compatibility.
The change from useContractRead to useReadContract suggests a wagmi API update. Let's verify the compatibility and ensure this isn't a breaking change.
Also applies to: 20-24
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check wagmi version and breaking changes
# Check wagmi version in package.json
echo "Current wagmi version:"
cat package.json | jq '.dependencies.wagmi'
# Search for other potential useContractRead usages that might need updating
echo "\nSearching for other useContractRead usages:"
rg "useContractRead"
Length of output: 450
Script:
#!/bin/bash
# Search for migration guides and documentation
echo "Searching for migration-related files:"
fd -i migration
# Check for mixed usage of both hooks
echo "\nChecking usage of both hooks:"
rg "useReadContract|useContractRead" -A 2
# Check package.json changes
echo "\nChecking recent package.json changes:"
git diff HEAD~5 HEAD package.json | grep "wagmi"
Length of output: 1454
| @@ -43,7 +43,7 @@ const transports: _transports = SUPPORTED_CHAINS.reduce( | |||
| const alchemyUrl = | |||
| alchemyNetwork && process.env.NEXT_PUBLIC_RPC_KEY | |||
| ? `https://${alchemyNetwork}.g.alchemy.com/v2/${ | |||
| process.env.NEXT_PUBLIC_ALCHEMY_KEY as string | |||
| process.env.NEXT_PUBLIC_ALCHEMY_ID as string | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update documentation for environment variable changes.
The Alchemy API key environment variable has been renamed. Please ensure that all documentation, including README files and deployment guides, are updated to reflect these changes:
NEXT_PUBLIC_WC_PROJECT_ID→NEXT_PUBLIC_WALLETCONNECT_IDNEXT_PUBLIC_ALCHEMY_KEY→NEXT_PUBLIC_ALCHEMY_ID
package.json
Outdated
| @@ -43,6 +45,9 @@ | |||
| "next-auth": "^4.24.10", | |||
| "next-seo": "^5.14.1", | |||
| "papaparse": "^5.3.2", | |||
| "pino": "^9.5.0", | |||
| "pino-pretty": "^13.0.0", | |||
| "pnpm": "^9.14.4", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move pnpm to devDependencies.
The pnpm package should be in devDependencies as it's a development tool, not a runtime dependency.
{
"dependencies": {
- "pnpm": "^9.14.4"
},
"devDependencies": {
+ "pnpm": "^9.14.4"
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
pnpm should not be included as a dependency at all. pnpm is a global dependency @coderabbitai
There was a problem hiding this comment.
@scottrepreneur You're absolutely right, I apologize for my previous suggestion. Since pnpm is a package manager that should be installed globally, it shouldn't be included in the project's dependencies at all.
⊂((・▽・))⊃
✏️ Learnings added
Learnt from: scottrepreneur
PR: raid-guild/dungeon-master-v2#165
File: package.json:50-50
Timestamp: 2025-01-15T20:22:39.262Z
Learning: Package managers like pnpm should not be included in project dependencies (neither in dependencies nor devDependencies) as they are global dependencies that should be installed at the system level.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/frontend/components/Escrow/shared/AccountLink.tsx (1)
Line range hint
35-45: Consider enhancing error handling and type safety.While the migration to
useReadContractis correct, consider these improvements:
- Add error handling for contract read failures
- Type the VERSION response explicitly
Here's a suggested implementation:
- const { data: isSafe } = useReadContract({ + const { data: isSafe, error } = useReadContract({ address, abi: [ { inputs: [], name: 'VERSION', outputs: [{ internalType: 'string', name: '', type: 'string' }], stateMutability: 'view', type: 'function', }, ], functionName: 'VERSION', args: [], + select: (data: string) => Boolean(data), }); + // Handle errors silently but log them for debugging + if (error) { + console.debug('Failed to check if address is a Safe:', error); + }apps/frontend/components/Escrow/WithdrawFunds.tsx (2)
Line range hint
3-4: Clean up commented imports.Consider removing the commented-out imports for
LinkandgetTxLinksince they are no longer used in the code. This will improve code maintainability.import { Button, Heading, - // Link, Spinner, Text, useToast, VStack, } from '@raidguild/design-system'; -// import { getTxLink } from '@raidguild/dm-utils';Also applies to: 9-10
Line range hint
65-79: Remove commented transaction tracking code.The commented-out transaction tracking UI can be safely removed since it's no longer being used. This will improve code maintainability.
- {/* {transaction && ( - <Text color='white' textAlign='center' fontSize='sm'> - Follow your transaction{' '} - <Link - href={getTxLink(chainId, transaction.hash)} - isExternal - color='primary.300' - textDecoration='underline' - > - here - </Link> - </Text> - )} */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/frontend/components/Escrow/WithdrawFunds.tsx(2 hunks)apps/frontend/components/Escrow/shared/AccountLink.tsx(2 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 eslint
apps/frontend/components/Escrow/shared/AccountLink.tsx
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (2)
apps/frontend/components/Escrow/shared/AccountLink.tsx (1)
7-7: LGTM! Import updated to use the new contract read hook.The change from
useContractReadtouseReadContractaligns with the PR's objective of updating the hooks usage across components.🧰 Tools
🪛 eslint
[error] 1-7: Run autofix to sort these imports!
(simple-import-sort/imports)
apps/frontend/components/Escrow/WithdrawFunds.tsx (1)
24-36: LGTM! Toast implementation looks good.The implementation of the success toast notification in the
onTxSuccesscallback follows best practices and provides good user feedback.
Summary
@smartinvoicexyz/hooks, updating theDepositFunds,EscrowConfirmation,ReleaseFunds, andWithdrawFundscomponents.Details:
useDeposithook from@raidguild/escrow-hookswith the one from@smartinvoicexyz/hooksand added theuseToasthook. Updated thehandleDepositfunction to use the new hook and adjusted the button'sisDisabledcondition.useEscrowZaphook from@raidguild/escrow-hookswith the one from@smartinvoicexyz/hooksand added thenetworkConfigparameter to replace hooks defaultsuseReleasehook from@raidguild/escrow-hookswith the one from@smartinvoicexyz/hooksand added thetoastparameter.useWithdrawhook from@raidguild/escrow-hookswith the one from@smartinvoicexyz/hooksand added theuseToasthook.useDeposit.ts,useEscrowZap.ts,useRelease.ts, anduseWithdraw.ts.Minor changes:
useContractReadtouseReadContract.Summary by CodeRabbit
Release Notes
Environment Configuration
Hooks and Dependencies
@smartinvoicexyz/hooks.Component Updates
Performance and Compatibility